feat: outbound file attachments (addresses #355, opt-in, rate-limited) by DrVictorChen · Pull Request #401 · openabdev/openab

DrVictorChen · 2026-04-16T14:28:32Z

Description

Closes #298. Addresses #355.

Agents running through OpenAB can receive files from users, but have no native path to send files back to the chat. This PR adds an opt-in, config-driven, rate-limited pathway: agents include ![alt](/path/to/file) markdown in their reply, OpenAB validates the path, uploads the file as a native attachment, and strips the marker from the visible text.

Supersedes #300. That PR proposed the same feature but was deferred pending the five security items listed in #355. This PR addresses all five plus one self-imposed hardening.

Changes

New platform-agnostic modules

src/media.rs::extract_outbound_attachments(text, &OutboundConfig) — regex extraction + canonicalization + allowlist + size/count validation.
src/outbound_rate.rs::OutboundRateLimiter — HashMap<channel_key, VecDeque<Instant>> 60-second sliding window, auto-prunes on each call.

ChatAdapter trait

New send_file_attachments(channel, paths) method with a default no-op, so adapters that don't support native file upload silently drop the list instead of erroring. DiscordAdapter overrides to upload via serenity CreateAttachment::path + CreateMessage::add_file. Slack / future platforms just override the method.

Wiring

AdapterRouter::stream_prompt — after the final text edit: extract markers → rate-admit → send_file_attachments with whatever the limiter granted.

Config (opt-in, top-level [outbound])

[outbound]
enabled = false                            # opt-in, disabled by default
allowed_dirs = ["/tmp/", "/var/folders/"]  # canonicalized at send time
max_file_size_mb = 25                      # matches Discord limit
max_per_message = 10                       # per agent response
max_per_minute_per_channel = 30            # sliding-window flood guard

Security checklist from #355

#	Requirement	Implementation
1	Symlink resolution (`canonicalize`)	`std::fs::canonicalize()` before allowlist check
2	Path traversal (`..`)	covered by the same canonicalize
3	Configurable `allowed_dirs`	`[outbound].allowed_dirs` in `config.toml`
4	Opt-in default	`OutboundConfig::default().enabled == false`
5	Rate limiting	per-message cap + per-channel per-minute sliding window
6 (extra)	Component-wise allowlist check	`Path::starts_with` on canonical paths, not `str::starts_with` (blocks `/tmp-fake/x`)

Markers that fail validation stay in the displayed text so the user sees what the agent tried — follows the Hermes Agent fallback-chain pattern rather than silent drop.

Verified

cargo build passes
cargo test — 54 passed (13 new: 8 in media::outbound_tests, 5 in outbound_rate::tests)
cargo clippy --all-targets -- -D warnings -A clippy::manual_contains clean
Four test scenarios covered end-to-end:
- happy path (/tmp/ file, native upload)
- symlink escape (ln -s /etc/hosts /tmp/x.png → blocked)
- path traversal (![x](/tmp/../etc/hosts) → blocked)
- rate-limit flood (5 requested vs 3 remaining slots → 3 granted, 2 dropped with warn)

Not in this PR

The Makefile macOS AMFI codesign fix bundled into feat: outbound image/file attachments from agent → Discord #300 — will be a separate PR per masami-agent's suggestion on that review.

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1493297320622821718

🤖 Generated with Claude Code

…te-limited) Closes openabdev#298. Addresses security items 1-5 in openabdev#355 (the follow-up requirements issue filed against the previous attempt in openabdev#300). ## What this ships Agents running through OpenAB can now include `![alt](/path/to/file)` markdown in their responses. OpenAB detects the marker, validates the path, uploads the file as a native chat attachment, and strips the marker from the displayed text. The feature is disabled by default — operators must opt in via `[outbound]` in `config.toml`. ## Architecture (adapter-agnostic) - `src/media.rs::extract_outbound_attachments(text, &OutboundConfig)` — platform-independent extraction + validation. - `src/outbound_rate.rs::OutboundRateLimiter` — per-channel sliding 60-second window enforcing `max_per_minute_per_channel`. - `ChatAdapter::send_file_attachments(channel, paths)` — new trait method with a default no-op. `DiscordAdapter` overrides to upload via serenity `CreateAttachment::path()` + `CreateMessage::add_file()`. Slack / future platforms can override as needed. - `AdapterRouter::stream_prompt` — after the final text edit: extract markers → apply rate limit → call `send_file_attachments`. ## Security items from openabdev#355 | # | Item | Implementation | |---|---|---| | 1 | Symlink resolution | `std::fs::canonicalize()` before allowlist check | | 2 | Path traversal (`..`) | same canonicalize resolves `..` components | | 3 | Configurable `allowed_dirs` | `[outbound].allowed_dirs` in config.toml | | 4 | Opt-in (`enabled = false` default) | `OutboundConfig::default().enabled == false` | | 5 | Rate limiting | per-message cap + per-minute sliding-window per channel | Allowlist is canonicalized at send time so macOS's `/tmp → /private/tmp` symlink causes no spurious blocks. Comparison is component-wise via `Path::starts_with`, not string prefix. Reply text for attempts that fail validation (blocked, too large, missing) keeps the marker in the text so the user can see what was tried. ## Tests 8 new in `media::outbound_tests`: - disabled_by_default_is_noop - enabled_extracts_tmp_file - blocks_non_allowlisted_path - blocks_symlink_escape - blocks_path_traversal - respects_custom_allowed_dirs - enforces_max_size_bytes - enforces_max_per_message - strips_multiple_valid_markers 5 new in `outbound_rate::tests`: - admits_up_to_limit_then_blocks_within_window - partial_admit_when_some_remaining_slots - channels_are_independent - zero_requested_or_zero_limit_grants_zero - prunes_entries_older_than_window All 54 tests pass. Clippy clean with `-D warnings -A clippy::manual_contains`. ## Config ```toml [outbound] enabled = false allowed_dirs = ["/tmp/", "/var/folders/"] max_size_bytes = 26214400 # 25 MB max_per_message = 10 max_per_minute_per_channel = 30 ``` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

masami-agent

PR Review: #401

Summary

Problem: Agents can receive files from users but have no native path to send files back to chat. Closes #298, addresses all security requirements from #355.
Approach: ![alt](/path) markdown markers in agent responses → regex extraction → canonicalize + allowlist + size validation → native upload via ChatAdapter::send_file_attachments. Opt-in via [outbound] config. Per-channel sliding-window rate limiter in a separate module.
Risk level: Medium (opens a filesystem → chat channel pathway, but the security model is solid)

Core Assessment

Problem clearly stated: ✅ (linked #298, #355, supersedes #300)
Approach appropriate: ✅ (platform-agnostic extraction, trait method with default no-op, standalone rate limiter)
Alternatives considered: ✅ (PR description references #300 and explains why it was deferred)
Best approach for now: ✅ (clean separation of concerns, opt-in by default)

Findings

Architecture — well-structured:

media.rs — platform-agnostic extraction + validation (no Discord dependency)
outbound_rate.rs — standalone rate limiter with HashMap<String, VecDeque<Instant>> sliding window
ChatAdapter trait — send_file_attachments with default no-op (Slack silently drops, no breakage)
AdapterRouter — wiring only, no business logic leakage

Security checklist from #355 — all 5 items addressed:

#	Requirement	Status
1	Symlink resolution (`canonicalize`)	✅
2	Path traversal (`..`)	✅
3	Configurable `allowed_dirs`	✅
4	Opt-in default	✅
5	Rate limiting	✅
6 (extra)	Component-wise `Path::starts_with`	✅

Review Summary

🔴 Blockers

Branch is 29 commits behind main — must rebase before merge. The Cargo.lock diff will likely conflict. This is a hard blocker per repo merge rules.

💬 Questions

canonicalize_allowlist() is called on every extract_outbound_attachments() invocation — this does filesystem I/O (std::fs::canonicalize) on each agent response. Was caching considered? For the default 2 dirs the cost is negligible, but worth documenting the trade-off or adding a note on why runtime canonicalization was preferred over startup-time caching (e.g. dirs could be mounted/unmounted at runtime).

🔧 Suggested Changes

Discord upload is sequential per file — the for path in paths loop in discord.rs uploads one file at a time. serenity supports a single CreateMessage with multiple add_file() calls, which would reduce API calls and be more atomic. Not blocking, but a nice improvement.
Config field naming — config uses max_file_size_mb but the method is max_size_bytes(). Consider max_file_size_bytes() to mirror the config field more closely, or add a doc comment on the method noting the MB → bytes conversion.

ℹ️ Info

The Cargo.lock change (0.7.6 → 0.7.7) is not a version bump by this PR — it fixes a pre-existing inconsistency on main where Cargo.toml is already 0.7.7 but Cargo.lock still says 0.7.6.
Slack adapter correctly inherits the default no-op send_file_attachments — no changes needed.
Tests are solid: 8 in media::outbound_tests + 5 in outbound_rate::tests, covering happy path, symlink escape, path traversal, custom allowlist, size limit, per-message cap, multi-file stripping, and rate limiter edge cases.

⚪ Nits

config.toml.example comment says 25 MB is Discord's limit — Discord's actual limit varies by boost level (8/25/50/100 MB). The default of 25 MB is a reasonable safe choice; the comment could note this nuance.

Verdict

COMMENT — code quality and security model are strong. The branch being 29 commits behind main is a hard blocker. After rebase (watch for Cargo.lock conflicts), this should be a clean approve. Nice work addressing all the #355 security items and splitting the Makefile fix into a separate PR as suggested.

DrVictorChen requested a review from thepagent as a code owner April 16, 2026 14:28

github-actions bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label Apr 16, 2026

DrVictorChen mentioned this pull request Apr 16, 2026

feat: outbound image/file attachments from agent → Discord #300

Closed

4 tasks

github-actions bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label Apr 16, 2026

thepagent self-assigned this Apr 16, 2026

masami-agent reviewed Apr 17, 2026

View reviewed changes

thepagent added the needs-rebase label Apr 18, 2026

chaodu-agent added the pending-contributor label Apr 18, 2026

github-actions bot added needs-rebase pending-contributor and removed needs-rebase pending-contributor labels Apr 18, 2026

github-actions bot added needs-rebase pending-contributor and removed pending-contributor labels Apr 19, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: outbound file attachments (addresses #355, opt-in, rate-limited)#401

feat: outbound file attachments (addresses #355, opt-in, rate-limited)#401
DrVictorChen wants to merge 1 commit intoopenabdev:mainfrom
DrVictorChen:feat/outbound-pr300-v2

DrVictorChen commented Apr 16, 2026 •

edited

Loading

Uh oh!

masami-agent left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

DrVictorChen commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Security checklist from #355

Verified

Not in this PR

Uh oh!

masami-agent left a comment

Choose a reason for hiding this comment

PR Review: #401

Summary

Core Assessment

Findings

Review Summary

🔴 Blockers

💬 Questions

🔧 Suggested Changes

ℹ️ Info

⚪ Nits

Verdict

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DrVictorChen commented Apr 16, 2026 •

edited

Loading